Skip to content
This repository was archived by the owner on Apr 26, 2026. It is now read-only.

fix: add shell:true for Windows .cmd file spawn#9

Open
nbalzotti wants to merge 2 commits into
unixfox:masterfrom
nbalzotti:fix/windows-spawn-shell
Open

fix: add shell:true for Windows .cmd file spawn#9
nbalzotti wants to merge 2 commits into
unixfox:masterfrom
nbalzotti:fix/windows-spawn-shell

Conversation

@nbalzotti

Copy link
Copy Markdown

Problem

On Windows, the Claude CLI is installed as claude.cmd (a batch file wrapper). Node.js's child_process.spawn() requires shell: true to execute .cmd files — without it, spawn throws EINVAL silently.

The plugin catches this error in the session manager but only logs it at debug level (which is disabled by default). The result: the CLI process never starts, OpenCode receives an empty response, and Claude Code appears to "do nothing."

Fix

Add shell: process.platform === "win32" to both spawn() call sites:

  • session-manager.ts (persistent session spawning)
  • claude-code-language-model.ts (one-shot doGenerate calls)

The condition is platform-gated so Unix systems are unaffected.

Testing

Verified on Windows 11 (Node.js v24.13.0):

  • Before: spawn EINVAL → empty response → "Opus does nothing"
  • After: Claude Code Opus responds correctly in ~2.5s

nbalzotti and others added 2 commits April 14, 2026 01:19
On Windows, spawn() with a .cmd file path (e.g. claude.cmd) throws
EINVAL without shell:true. This causes the plugin to silently fail —
the CLI process never starts, and OpenCode receives an empty response.

Uses process.platform === "win32" to only enable shell mode on Windows,
avoiding unnecessary shell overhead on Unix.

Fixes both spawn sites: session-manager (persistent sessions) and
claude-code-language-model (doGenerate one-shot calls).
- Add mcpConfigPath to config/settings types, pass --mcp-config flag
- Filter empty text blocks in message-builder to prevent API errors
- Update LanguageModelV2Usage to new nested format (inputTokens.total/cacheRead)
- Replace empty user message with "Continue." to avoid cache_control errors

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@unixfox unixfox requested a review from Copilot April 16, 2026 02:03

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR primarily targets Windows compatibility for launching the Claude CLI by adjusting child_process.spawn() options, so .cmd shims can be executed successfully on win32. It also includes several additional functional changes (new config option, message-building behavior, and usage-shape changes) that expand the scope beyond the PR description.

Changes:

  • Enable shell: process.platform === "win32" for Claude CLI subprocess spawning in both persistent-session and one-shot execution paths.
  • Add an optional mcpConfigPath configuration value and plumb it through to CLI args (--mcp-config).
  • Modify prompt/message construction to drop empty text parts and send a synthetic "Continue." message when no content is produced; adjust usage object shaping/casting.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
src/types.ts Adds mcpConfigPath to public config/settings types.
src/session-manager.ts Uses shell on Windows for spawn; adds --mcp-config arg support.
src/message-builder.ts Skips empty text parts; changes fallback message to "Continue.".
src/index.ts Passes mcpConfigPath from provider settings into model config.
src/claude-code-language-model.ts Uses shell on Windows for spawn; passes mcpConfigPath; changes usage shaping and adds type casts.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/session-manager.ts
Comment on lines 53 to 57
const proc = spawn(cliPath, cliArgs, {
cwd,
stdio: ["pipe", "pipe", "pipe"],
shell: process.platform === "win32",
env: { ...process.env, TERM: "xterm-256color" },

Copilot AI Apr 16, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On Windows, enabling shell: true changes argument handling and can introduce command-injection/quoting issues if any CLI args (e.g., --model, paths) contain shell metacharacters or spaces. Consider avoiding shell unless the target is actually a .cmd shim (e.g., detect/resolve the real executable), or use a Windows-safe spawn helper that properly escapes arguments when invoking cmd.exe.

Copilot uses AI. Check for mistakes.
Comment on lines 200 to 206
const proc = spawn(this.config.cliPath, cliArgs, {
cwd,
stdio: ["pipe", "pipe", "pipe"],
shell: process.platform === "win32",
env: { ...process.env, TERM: "xterm-256color" },
})

Copilot AI Apr 16, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same concern as the session manager: shell: process.platform === "win32" can change quoting semantics and allow shell interpretation of arguments. Prefer a .cmd-specific execution path (or an escaping helper) so only the Windows shim case uses a shell and arguments remain safely escaped.

Suggested change
const proc = spawn(this.config.cliPath, cliArgs, {
cwd,
stdio: ["pipe", "pipe", "pipe"],
shell: process.platform === "win32",
env: { ...process.env, TERM: "xterm-256color" },
})
const isWindowsCmdShim =
process.platform === "win32" &&
/\.(cmd|bat)$/i.test(this.config.cliPath)
const escapeForCmdExe = (value: string): string => {
const escapedForQuotes = value.replace(/"/g, '""')
const escapedMetaChars = escapedForQuotes.replace(
/([()%!^"<>&|])/g,
"^$1",
)
return `"${escapedMetaChars}"`
}
const proc = isWindowsCmdShim
? spawn(process.env.ComSpec ?? "cmd.exe", [
"/d",
"/s",
"/c",
`${escapeForCmdExe(this.config.cliPath)} ${cliArgs
.map((arg) => escapeForCmdExe(arg))
.join(" ")}`,
], {
cwd,
stdio: ["pipe", "pipe", "pipe"],
shell: false,
env: { ...process.env, TERM: "xterm-256color" },
})
: spawn(this.config.cliPath, cliArgs, {
cwd,
stdio: ["pipe", "pipe", "pipe"],
shell: false,
env: { ...process.env, TERM: "xterm-256color" },
})

Copilot uses AI. Check for mistakes.
Comment thread src/types.ts
Comment on lines 1 to 15
export interface ClaudeCodeConfig {
provider: string
cliPath: string
cwd?: string
skipPermissions?: boolean
mcpConfigPath?: string
}

export interface ClaudeCodeProviderSettings {
cliPath?: string
cwd?: string
name?: string
skipPermissions?: boolean
mcpConfigPath?: string
}

Copilot AI Apr 16, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR introduces a new public config surface (mcpConfigPath) that isn’t mentioned in the PR description and isn’t documented in the README configuration section. Either update the PR description/docs accordingly, or split these changes into a separate PR to keep the Windows spawn fix scoped and reviewable.

Copilot uses AI. Check for mistakes.
Comment thread src/message-builder.ts
Comment on lines 137 to 145
if (content.length === 0) {
return JSON.stringify({
type: "user",
message: {
role: "user",
content: [{ type: "text", text: "" }],
content: [{ type: "text", text: "Continue." }],
},
})
}

Copilot AI Apr 16, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new behavior here (dropping empty text parts and emitting a synthetic "Continue." message) is a functional change unrelated to the Windows .cmd spawning fix described in the PR. Please either justify it in the PR description (including the motivating bug) and add coverage, or move it into a separate PR to avoid scope creep.

Copilot uses AI. Check for mistakes.
Comment on lines +147 to +149
inputTokens: { total: 0, noCache: 0, cacheRead: undefined, cacheWrite: undefined },
outputTokens: { total: 0, text: undefined, reasoning: undefined },
} as unknown as LanguageModelV2Usage,

Copilot AI Apr 16, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The usage shape is being changed and then forced through with as unknown as LanguageModelV2Usage, which bypasses type safety and may break downstream consumers expecting the previous runtime shape. Prefer constructing a LanguageModelV2Usage value that type-checks without double-casting (or keep the prior shape) and ensure the change is intentional/documented since it’s outside the PR’s stated scope.

Suggested change
inputTokens: { total: 0, noCache: 0, cacheRead: undefined, cacheWrite: undefined },
outputTokens: { total: 0, text: undefined, reasoning: undefined },
} as unknown as LanguageModelV2Usage,
inputTokens: 0,
outputTokens: 0,
totalTokens: 0,
},

Copilot uses AI. Check for mistakes.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants